Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added transform control and select entities GUI plugins #854

Merged
merged 41 commits into from
Aug 14, 2021
Merged

Added transform control and select entities GUI plugins #854

merged 41 commits into from
Aug 14, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jun 9, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

This PR is part of the consolidation between Scene 3D in ign-gazebo and ign-gui. This PR adds two independent plugin to select entities and transform control.

It builds on top of #813

Test it

ign gazebo --gui-config src/ign-gazebo/src/gui/gui.config rolling_shapes.sdf -v 4

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

chapulina and others added 3 commits May 12, 2021 14:08
@chapulina
Add scene manager GUI plugin that works with ign-gui's Scene3D
3a7d09c 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
cleanup
80161fb 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@ahcorde
Added tranform control and select entities GUI plugins
22454e9 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Jun 9, 2021
Merged
18 tasks
@ahcorde
Used new plugin
4465595 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Jun 18, 2021
Closed
ahcorde and others added 4 commits June 18, 2021 15:01
@ahcorde
Fix Key event class name
eda24b6 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina
Merge branch 'main' into chapulina/6/scene_manager
d0d7066 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Update to latest
5f94fe2 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@ahcorde
Some fixes to select entities plugin
b7922fc 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina June 22, 2021 17:13
ahcorde and others added 4 commits June 22, 2021 19:14
chapulina
chapulina reviewed Jun 25, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to compile this branch because I think I have the wrong combination of ign-gui branches.

The main high-level comment I have is whether it would be better to combine these 2 with the existing TransformControl plugin. I think all this logic should originally have been contained in that plugin, but we didn't have a mechanism for manipulating the 3D scene, so part of it ended up in Scene3D.

src/gui/plugins/select_entities/CMakeLists.txt Show resolved Hide resolved
src/gui/plugins/select_entities/SelectEntities.hh Outdated Show resolved Hide resolved
src/gui/plugins/transform_control_logic/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui/plugins/transform_control_logic/TransformControlLogic.hh Outdated Show resolved Hide resolved
src/gui/plugins/transform_control_logic/TransformControlLogic.cc Outdated Show resolved Hide resolved
src/gui/plugins/transform_control_logic/TransformControlLogic.cc Outdated Show resolved Hide resolved
src/gui/plugins/transform_control_logic/TransformControlLogic.cc Outdated Show resolved Hide resolved
include/ignition/gazebo/gui/GuiEvents.hh Outdated Show resolved Hide resolved
@ahcorde
Merge remote-tracking branch 'origin/chapulina/6/scene_manager' into …
27ee0d6 
…ahcorde/plugin/selectEntities_transformControl
@ahcorde
Added feedback
6bb0bce 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
make linters happy
e00ee2e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Merge branch 'main' into chapulina/6/scene_manager
c3095a2
@chapulina chapulina added the 🏯 fortress Ignition Fortress label Aug 4, 2021
@chapulina
Merge branch 'main' into chapulina/6/scene_manager
c741662
@chapulina
Don't make it official yet
fbceade 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Merge branch 'chapulina/6/scene_manager' of ssh://github.com/ignition…
f27887a 
…robotics/ign-gazebo into chapulina/6/scene_manager

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
alphabetize
21e1b7b 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
merged from chapulina/6/scene_manager
20c0e0b 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@ahcorde ahcorde self-assigned this Aug 10, 2021
@chapulina
Copy link
Contributor

@ahcorde . I started reviewing this and making some changes in ahcorde/plugin/selectEntities_transformControl...chapulina/select_transform. I thought they would be mainly trivial changes, updating style, namespaces and whatnot, as well as combining TransformControl with TransformControlLogic. But then I realized that some features are not working yet on for transform controls:

  • Snapping (the keyEvent never has modifiers like Control, so it's not possible to snap while dragging)
  • In select mode, select an entity, then change to translate mode. The transform controls should attach to the last selected entity, but it doesn't attach to anything.

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 11, 2021

Requires gazebosim/gz-gui#268

@ahcorde
make linters happy
cbf5c00 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Remove transform control logic plugin
5282927 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Aug 11, 2021
ahcorde and others added 3 commits August 12, 2021 12:34
@ahcorde
Fixed compilation error
ee7784c 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Remove traces
73b9e4e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina
Documentation and style
caf08a9 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
chapulina
chapulina reviewed Aug 13, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed some documentation and style updates in caf08a9.

I noticed a change in behaviour. Pressing Shift is toggling between world and local frames, but the correct behaviour is to use the world frame while the Shift key is being held, and going back to local when the key is released, as explained in the tutorial:

https://ignitionrobotics.org/docs/edifice/manipulating_models#align-to-world-frame

src/gui/plugins/transform_control/TransformControl.cc Outdated Show resolved Hide resolved
src/gui/plugins/scene_manager/GzSceneManager.cc Outdated Show resolved Hide resolved
@ahcorde
Fixed keyRelease event
e6e3e16 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 13, 2021

I noticed a change in behaviour. Pressing Shift is toggling between world and local frames, but the correct behaviour is to use the world frame while the Shift key is being held, and going back to local when the key is released, as explained in the tutorial:

I was using the old KeyEvent. Solved e6e3e16

@ahcorde
Fixed frozen scene
ae18bad 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fix seg fault
0a754f4 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Renamed TransformControlModeActive to TransformControlModeActive
4cec6b2 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
make linters happy
4279b67 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde changed the title Added tranform control and select entities GUI plugins Added transform control and select entities GUI plugins Aug 13, 2021
@chapulina
Copy link
Contributor

Everything seems to be working well for me now, thanks! I just have this final question: e6e3e16#commitcomment-54864874

@ahcorde
Fixed key event for legacy mode
6ab0c3d 
Signed-off-by: ahcorde <ahcorde@gmail.com>
chapulina
chapulina approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! We just need some fresh ign-gui nightlies to go with this. I'm on it.

Triggered: https://build.osrfoundation.org/job/ign-gui6-debbuilder/

@codecov
Copy link

codecov bot commented Aug 13, 2021 •
edited
Loading

Codecov Report

Merging #854 (943679d) into main (eaf7aab) will decrease coverage by 0.90%.
The diff coverage is 4.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #854      +/-   ##
==========================================
- Coverage   65.43%   64.53%   -0.91%     
==========================================
  Files         246      246              
  Lines       19433    19729     +296     
==========================================
+ Hits        12716    12732      +16     
- Misses       6717     6997     +280
Impacted Files Coverage Δ
include/ignition/gazebo/gui/GuiEvents.hh 0.00% <0.00%> (ø)
src/gui/plugins/scene3d/Scene3D.cc 10.79% <0.00%> (-0.01%) ⬇️
src/rendering/SceneManager.cc 35.13% <ø> (-0.07%) ⬇️
.../gui/plugins/transform_control/TransformControl.cc 7.23% <4.45%> (-11.92%) ⬇️
src/gui/plugins/plot_3d/Plot3D.cc 47.82% <0.00%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaf7aab...943679d. Read the comment docs.

@chapulina
Merge branch 'main' into ahcorde/plugin/selectEntities_transformControl
b822910
@chapulina
revert unused blockUpdate
eb51a6c 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Merge branch 'ahcorde/plugin/selectEntities_transformControl' of ssh:…
943679d 
…//github.com/ignitionrobotics/ign-gazebo into ahcorde/plugin/selectEntities_transformControl

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Aug 13, 2021
@chapulina chapulina merged commit fc35de3 into main Aug 14, 2021
@chapulina chapulina deleted the ahcorde/plugin/selectEntities_transformControl branch August 14, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@iche033 iche033 Awaiting requested review from iche033 iche033 is a code owner

Assignees

@ahcorde ahcorde

Labels
🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ahcorde @chapulina